-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CRC part is also possible with v0/v1 #110
Conversation
FFmpeg permits to add CRC data after a v0/v1 stream, so we document it.
The implementation does not detect the CRC in these files. So specification and decoder implementation currently agree that this is not correct. |
Having the decoder ignoring it is not important IMO (does not impact other decoders) IMO the goal when doing this specification is to document files in the wild, and here such files are in the wild, decoding is fine with such files, I don't like the idea to consider such files currently created by FFmpeg as invalid when there is no decoding issue with it. Just to be clear: do you say that FFmpeg may currently create, with |
If the decoder doesnt decode it and the specification doesnt match it. Then i think there can be no question that it is invalid currently. Also I do not think the specification should describe every ffv1 file out there no matter what buggy version or potentially intentionally contradictionary parameters it was encoded with. And declare every such file valid. |
I answered too vaguely yesterday. So for your sentence: "So specification and decoder implementation currently agree that this is not correct" no they don't agree.
Specification adds a constraint compared to decoder.
IMO out of topic: permitting CRC in v0/v1 does not mandate handling CRC.
Yes, because you think that FFmpeg does not create invalid bitstream because the decoder decodes it correctly, but the specification explicitly says that bitstream with trailing bytes are invalid (it does not permit it).
I disagree, right, because the current spec says that bitstream created by "FFmpeg -level 1 -slicecrc 1" are invalid.
My patch does not make implementations more complex: both encoder and decoder can ignore this patch (encoder does not create that, decoder ignores as FFmpeg decoder does currently. Nothing in spec mandates to compute CRC, whatever is the version, it is optional). I see 2 different issues: 1/ We need to decide (no choice "don't decide", no decision is a decision) if trailing bytes in a bitstream are valid or not valid (for v0/v1, I think we all agree it is invalid for v3) 2/ the only files I found with "reserved" bytes are the ones with CRC, and I don't see something against documenting that: implementation would not be more complex (this part can be ignored by encoders and decoders), and it adds the crc feature to v0/v1. Additionally, it permits in theory FFV1 fixers to fix by eliminating invalid decodings when testing (if trailing bytes are valid, fixer can no more consider bitstream modified for testing validity as invalid); note that in practice it does not prevent fixers to be aware that the only trailing bytes in the wild are CRC and suggest to user that bitstreams with trailing bytes not being CRC are not the ones he is looking for. Should I modify this PR (or new PR?) for putting "if( version <= 1 && remaining_bits_in_bitstream( NumBytes ) ) then reserved" instead of CRC so spec does not say that "FFmpeg -c ffv1 -slicecrc 1" creates invalid bitstreams? |
No, you mix things up here. The specification has to require both that trailing bytes must not be added by an encoder following "this" version as well as must require a decoder to ignore them. Thats required so that we can add fields in a "minor" update without breaking decodability with existing decoders. If you want to describe in some "non normative" part of the spec all kinds of broken files (there are many more than this) i surely dont mind but i do not agree to add implementation bugs into the normative spec so that we in the future start creating such files and all implementations must by made buggy to be compliant. |
I disagree: I just explained what is the current status of each part (decoder and spec) You sentences are one of the change we could do, but this is not the only possibility. I suggest a possibility to add CRC feature to v0/v1 spec as it is currently in FFmpeg encoder, as it does not hurt, that's all. I didn't expect that it is a problem, as the decoder ignores such bits and it does not create any complexity (decoders not using the feature just ignore bits as done today, decoders willing to use the feature test if 5 bytes are remaining and set "ec" flag to 1 if it is the case). Just an easy/lazy way to add crc feature to v0/v1.
I don't want to add "non normative" part, I just want to be clear about FFmpeg creating for the last few years invalid streams or not, and/or to add (normative) features when it does not hurt. Here, I don't see what is blocking for that unexpected feature from FFmpeg becomes normative, so I suggested first to make it normative.
You mean "ec" flag? True, but I detect "implicitly" its presence with the patch. please check the patch for that. Not a problem. Anyway, I created #112 for meeting "The specification has to require both that trailing bytes must not be added by an encoder following "this" version as well as must require a decoder to ignore them" sentence. |
You are a bit stubborn. ;) The specification describes what is a valid file and how to decode it. The question about rencode for long term preservation. I dont want to jump to a hasty conclusion here and cause avoidable pain. But it does seem to me that reencoding these files if someone was so unfortunate to actually pick this unlucky combination of options ... yes it does make sense to reencode i think. I dont think it needs to be done immedeatly but maybe as part of some other maintaince or changes thats needed in the future ... The person wanted CRCs, the code wrote only half the syntax for CRCs, also the person asked for slicecrc but there are also no slices just one "frame". So this might not be what the person wanted. Its just a bug that resulted in half the CRC syntax to be written into the old version. OTOH if this is exactly what the user wanted then he maybe doesnt need to reencode but most of the tools might not be able to work with the CRCs as these are different. Also adding crc support into v0/v1 is alot of work nothing like this patch. It requires designing, implementing and testing of the detection and correction code, testcases, ... |
I have contradictory requests, like documenting what exists (files in the wild) and not documenting what exists (reject files not expected to be there even if FFmpeg created them). Note that I already accepted the idea to not add CRC in spec, see #112. I also create a FFV1 checker, and I see during implementation that the reference decoder and the spec are contradictory. A checker says "valid" or "invalid", not possible to say "ignore trailing bytes". So please indicate which one is the correct one (are trailing bytes valid or not valid), not saying which one is correct is not good for a spec (introduce doubts). Note that the goal of a spec is that a decoder could be implemented without looking at FFmpeg code. Here, a person relying on the spec may reject the files created by current version of "ffmpeg --level 0 -crc 1" even if FFmpeg accepts it, because the spec currently indicates that no bytes should appear after SliceContent in versions 0/1, so facing trailing bytes may indicate that something went wrong (exactly like if CRC is wrong in version 3). FFmpeg decoder code and spec do not match. Trashing the issue will not make it disappear.
FFmpeg has other bugs, there are 2 exceptions in the spec because the priority was to document FFmpeg bugs, I just do same here so I am surprised that the patch proposal is rejected.
Do you confirm that "ffmpeg --level 0 -crc 1" currently creates invalid files?
It is same with "ffmpeg -level 3 -slicecrc 1 -slices 1", could be considered as "no slice" too, and CRC is there anyway.
Already done: before sending the patch, I implemented it in my decoder (latest version of MediaConch tests that CRC is present then test it. In previous release, it was flagging the stream as invalid as trailing bytes were detected and it is forbidden by spec; can be changed to whatever in next version of MediaConch, but right now it works pretty well with that, with the millions of combinations from FFmpeg options I tested), and I tested that FFmpeg decoder is not impacted (it ignores the CRC). Note that my patch here (and my code) only accepts 40 bits, rejecting any other trailing bytes size, in order to limit use cases, but we could say "40 bits or more" if we want to keep the possibility to add features in versions 0/1 in the future. I document FFmpeg encoder behavior, nothing else.
It works, and IMO it makes lot of sense (expanding version 0/1 with CRC feature without breaking old decoders). But I don't use it myself, so I am fine if this idea is rejected. I actually just need to know if trailing bytes are accepted (FFmpeg decoder code) or not (FFV1 current specification).
Sorry, but still confusing for me, please be more explicit: do you confirm that "ffmpeg -level 0 -crc 1" currently creates invalid files or do you consider that such files are valid? I already accepted that the idea to keep current FFmpeg behavior (injecting CRC in version 0/1) is not OK, I don't argue anymore about that, but it does not remove the issue of mismatch between reference decoder and spec, please indicate which one is fine: decoder (so #112, modified or not depending of some tweaking, should be merged) or spec (so #112 is also rejected and we are clear that "ffmpeg -level 0 -crc 1" currently creates invalid files). |
Just for reference: there is no extension possible with current spec (as well with this patch, I didn't change want to change that here). And I don't think it would be a mess, there are lot of possibilities for handling that nicely, I could do some proposal if you change your mind about this PR (else #112 is enough as it is like current FFmpeg decoder code, people would think about that when they need to add extensions, I added a comment about buggy files for this reason) |
There is some sort of misunderstanding here. What i suggest is to document that there exist some files that include the CRC in v0/v1 and that decoders may want to support this but that it is not valid and that encoders must not generate such files. ill reply to the rest of your message too seperatly |
Define what "valid" means About "valid" if your checker checks that a stream is decodable by a compliant to spec 123 decoder thats different than if it checks if a stream is encoded correctly according to spec 123. |
I think it does create invalid files. But if these files are widespread we should support it in the decoder (which it already does) and make sure any implementation of the spec behaves reasonable with these files |
I disagree, as I think it is not bad to extend v0/v1 this way, so we could have hit two targets with one shot in a smooth manner, but I did not succeed to convince you despite the tests I have done for being sure there is no conflict, so let's focus on how to document the original issue (decoder/spec mismatch).
A decoder conforming to spec can decode a file conform to spec.
At the end of v0/v1 frame, see current patch proposal, it is indicated there.
Yes, if it is planned. Here, it is not the case (spec does not reserve bits for future extension).
I am not comfortable with something "invalid but decoder should decode", IMO the spec should just say what is valid (and if not valid, it is invalid). Maybe with a background, but not saying "this is invalid but you should decode them". If a decoder should decode such stream, the stream is expected to be considered valid IMO. semantic apart, let's be clear about what is authorized or not: which possibility is acceptable for you? 1/ Trailing bytes are acceptable in v0/v1 (for future extension of v0/v1) --> In that case, I already provided the patch: #112 (taking care of "-level 0 -slicecrc 1"), using reference decoder behavior, please merge it so we have coherency between decoder, talks, and spec. 2/ Trailing bytes are NOT acceptable in v0/v1 (except for "-level 0 -slicecrc 1") --> In that case, I adapt this PR for saying that the stream should be decoded ignoring trailing 40 bits if 40 bits are present. We are also clear that "future extension" is not possible (forbidden by spec), even if reference decoder currently authorize that by ignoring trailing bits (because other decoders may be less tolerant to streams not conforming to spec). |
FFmpeg permits to add CRC data after
SliceContent
in a v0/v1 stream (e.g../ffmpeg -f lavfi -i mandelbrot -t 1 -c ffv1 -slicecrc 1 a.mkv
), so we document it (for the moment, no extra data is expected after theSliceContent
part in v0/v1, so the stream may be considered as invalid).It is an implicit signaling (no
ec
metadata), so we detect the presence of CRC by checking if 5 bytes are remaining at the end of the frame.